Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change implementation to be modular #2

Closed
wants to merge 1 commit into from
Closed

Change implementation to be modular #2

wants to merge 1 commit into from

Conversation

knpwrs
Copy link

@knpwrs knpwrs commented Dec 30, 2012

This changes smokesignals to be modular in such a fashion that the same file can be loaded by CommonJS (node's require), AMD (RequireJS), or just included on a page.

@bentomas
Copy link
Owner

Hey good idea! Unfortunately, I'm not going to make the file any bigger for the people directly including it. Goal number 2 is to be as small as possible, and this makes it quite a big bigger (by 40%). I've tried very hard to make it as small as I possibly can, and won't be accepting any changes to the minified version that either A) don't make it smaller, or B) don't fix bugs.

I would, however, consider allowing another two files to be added (in addition to smokesignals.js and smokesignals.min.js--maybe call them smokesignals.mod.js and smokesignals.mod.min.js) for people trying to include it with a loader of some sort. But the way you have done it isn't the most efficient size-wise. At the very least these lines from an older version of this library use fewer characters:

(function (definition,undef) {
undef+='';
typeof module != undef ? module.exports = definition() :
typeof define != undef ? define(definition) :
this['smokesignals'] = definition()
})(function() {

(I saw no reason to check for the amd property since for the module check in the previous section we cannot do any additional checking either).

If you want to code that up , I'd merge that! Maybe also remove all the comments from this new file and have a comment pointing them to the canonical version of the lib (smokesignals.js).

Thanks for being interested in making this library better and more versatile!

@bentomas bentomas closed this Dec 30, 2012
@knpwrs
Copy link
Author

knpwrs commented Dec 30, 2012

Just curious, if you previously supported AMD why did you remove it?

@bentomas
Copy link
Owner

bentomas commented Feb 5, 2013

Woops! Somehow missed this comment last month!

To make the library considerably smaller. And no one complained.

Plus module loaders like that seem anathema to the philosophy of making libraries as small as possible. The require.js file is 14.6KB minified or 9.5 gzipped. That's a lot of page weight for something that can be fairly easily done with a simple <script src=""></script> and a precompiler. I'm not saying they aren't useful, I'm just saying I wasn't willing to sacrifice the goals of this project to support them.

But like I said, I'd be happy to add another file to the project which adds that functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants